feat: add support for VPC direct connect#299
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for Direct VPC Egress by adding a new NetworkInterface dataclass and a network_interface option to RuntimeOptions and global options. It also adds a vpc_egress alias for vpc_connector_egress_settings, updates manifest generation to handle these new settings, and includes comprehensive unit tests. The review feedback suggests a valuable simplification: removing the vpc_egress alias from resettable_options to avoid default RESET_VALUE population, which in turn simplifies the precedence logic in _endpoint() and allows the removal of an unnecessary test assertion.
| "service_account", | ||
| "vpc_connector", | ||
| "vpc_connector_egress_settings", | ||
| "vpc_egress", | ||
| "network_interface", | ||
| ] |
There was a problem hiding this comment.
Since vpc_egress is an alias for vpc_connector_egress_settings, it does not need to be included in resettable_options. Including it causes it to be populated with RESET_VALUE by default when preserve_external_changes is False, which complicates the logic in _endpoint() and can lead to bugs where explicit overrides are ignored. Removing it allows for a much simpler and more robust override check.
"service_account",
"vpc_connector",
"vpc_connector_egress_settings",
"network_interface",
]| vpc_egress = options.vpc_connector_egress_settings | ||
| if options.vpc_egress is not None and ( | ||
| not isinstance(options.vpc_egress, _util.Sentinel) or vpc_egress is None | ||
| ): | ||
| vpc_egress = options.vpc_egress |
There was a problem hiding this comment.
With vpc_egress removed from resettable_options, we can simplify this logic significantly. If options.vpc_egress is not None, it means the user explicitly specified it (either as a valid VpcEgressSetting or as RESET_VALUE). Therefore, we can let it take precedence over vpc_connector_egress_settings directly without complex type checks.
vpc_egress = options.vpc_connector_egress_settings
if options.vpc_egress is not None:
vpc_egress = options.vpc_egress| assert options_asdict["network_interface"] is options.RESET_VALUE, ( | ||
| "network_interface should be RESET_VALUE" | ||
| ) | ||
| assert options_asdict["vpc_egress"] is options.RESET_VALUE, "vpc_egress should be RESET_VALUE" |
There was a problem hiding this comment.
Since vpc_egress is no longer in resettable_options, it will not be populated with RESET_VALUE in options_asdict by default. We should remove this assertion.
| assert options_asdict["network_interface"] is options.RESET_VALUE, ( | |
| "network_interface should be RESET_VALUE" | |
| ) | |
| assert options_asdict["vpc_egress"] is options.RESET_VALUE, "vpc_egress should be RESET_VALUE" | |
| assert options_asdict["network_interface"] is options.RESET_VALUE, ( | |
| "network_interface should be RESET_VALUE" | |
| ) |
2 participants
Fixes #275
This PR adds support for VPC direct connect. These additions help to maintain parity between the Python and JS SDKs.
Testing
Deployed the following function:
Ran
gcloud run services describe vpcprobe --region=us-central1 --format exportOutput included:
Then for the reset path:
Deployed:
Ran
gcloud run services describe vpcprobe --region=us-central1 --format exportThe following were no longer present in the output